Pull Request Test Coverage Report for Build 13239732856

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.05%) to 92.299%

Totals
Change from base Build 13237729498:	0.05%
Covered Lines:	9288
Relevant Lines:	10063

💛 - Coveralls

Hey @sjrl I've tested your implementation a little bit

Performance:
When running the tests test_large_standard_case and test_large_side_by_side_case, I observed the following timings:

14.66s call     test/components/preprocessors/test_csv_document_splitter.py::TestCSVDocumentSplitterV2::test_large_side_by_side_case
14.48s call     test/components/preprocessors/test_csv_document_splitter.py::TestCSVDocumentSplitterV2::test_large_standard_case

which, on average, is a 2x improvement in terms of speed vs the previous BFS approach. 💪 good job

Correctness
Consider the test case test_complex_bridging

    def test_complex_bridging(self) -> None:
        """
        Rows bridging from left to right => BFS splits each row into left block & right block.
        """
        csv_data = """ID,LeftVal,,,RightVal,Extra
1,Hello,,,World,Joined
2,StillLeft,,,StillRight,Bridge

A,B,,,C,D
E,F,,,G,H
"""
        splitter = CSVDocumentSplitterV2(row_split_threshold=1, column_split_threshold=1)
        result = splitter.run([Document(content=csv_data)])
        docs = result["documents"]
        assert len(docs) == 4
        block_texts = [doc.content for doc in docs]
        assert any("ID,LeftVal" in text for text in block_texts)
        assert any("Hello" in text for text in block_texts)
        assert any("World,Joined" in text for text in block_texts)
        assert any("StillLeft" in text for text in block_texts)
        assert any("StillRight,Bridge" in text for text in block_texts)
        assert any("A,B" in text for text in block_texts)
        assert any("C,D" in text for text in block_texts)
        assert any("E,F" in text for text in block_texts)
        assert any("G,H" in text for text in block_texts)

In this scenario, the expected output should yield 4 documents rather than 2. skip_blank_lines in pandas is default True and this causes the problem. The same issue occurs for the test test_empty_rows_and_side_tables

Usability
Overall, I find your implementation to be more intuitive and easier to understand

Hey @sjrl I've tested your implementation a little bit

Performance: When running the tests test_large_standard_case and test_large_side_by_side_case, I observed the following timings:

14.66s call     test/components/preprocessors/test_csv_document_splitter.py::TestCSVDocumentSplitterV2::test_large_side_by_side_case
14.48s call     test/components/preprocessors/test_csv_document_splitter.py::TestCSVDocumentSplitterV2::test_large_standard_case

which, on average, is a 2x improvement in terms of speed vs the previous BFS approach. 💪 good job

Correctness Consider the test case test_complex_bridging

    def test_complex_bridging(self) -> None:
        """
        Rows bridging from left to right => BFS splits each row into left block & right block.
        """
        csv_data = """ID,LeftVal,,,RightVal,Extra
1,Hello,,,World,Joined
2,StillLeft,,,StillRight,Bridge

A,B,,,C,D
E,F,,,G,H
"""
        splitter = CSVDocumentSplitterV2(row_split_threshold=1, column_split_threshold=1)
        result = splitter.run([Document(content=csv_data)])
        docs = result["documents"]
        assert len(docs) == 4
        block_texts = [doc.content for doc in docs]
        assert any("ID,LeftVal" in text for text in block_texts)
        assert any("Hello" in text for text in block_texts)
        assert any("World,Joined" in text for text in block_texts)
        assert any("StillLeft" in text for text in block_texts)
        assert any("StillRight,Bridge" in text for text in block_texts)
        assert any("A,B" in text for text in block_texts)
        assert any("C,D" in text for text in block_texts)
        assert any("E,F" in text for text in block_texts)
        assert any("G,H" in text for text in block_texts)

In this scenario, the expected output should yield 4 documents rather than 2. skip_blank_lines in pandas is default True and this causes the problem. The same issue occurs for the test test_empty_rows_and_side_tables

Usability Overall, I find your implementation to be more intuitive and easier to understand

Thanks for taking a look @alex-stoica! I’ll make the changes you suggest and add the test cases for this.

LGTM - we should now also have an issue to keep track the documentation for this new component

Opened issue here: #8835